Limit number of firewall rules per VPC #5914

david-crespo · 2024-06-18T21:48:01Z

Closes #5662
Closes #6032

Max firewall rules
Enforce max length on filters (hosts, protocols, and ports) and targets too
Improve doc comments on firewall rules types and API endpoint

karencfv · 2024-06-18T22:18:06Z

nexus/db-model/src/vpc_firewall_rule.rs

@@ -264,6 +265,20 @@ impl VpcFirewallRule {
    }
 }

+const MAX_FIREWALL_RULES: usize = 32;


Is this an arbitrary number? Perhaps it'd be good to have a comment that explains why we chose this limit?

This number feels way too low. I would expect a value in the hundreds at least, especially since we don't have tags for grouping firewall rules.

Yeah, definitely. Was going to ask about the number next. How about 512?

karencfv

Thanks for this PR! It'll really help set customer's expectations on what is allowed. Could you add documentation about these limits to the relevant API endpoints?

karencfv · 2024-06-30T23:25:15Z

nexus/db-model/src/vpc_firewall_rule.rs

+        ensure_max_len(&rule.targets, "targets", MAX_FW_RULE_PARTS)?;
+
+        if let Some(hosts) = rule.filters.hosts.as_ref() {
+            ensure_max_len(&hosts, "filters.hosts", MAX_FW_RULE_PARTS)?;
+        }
+        if let Some(ports) = rule.filters.ports.as_ref() {
+            ensure_max_len(&ports, "filters.ports", MAX_FW_RULE_PARTS)?;
+        }
+        if let Some(protocols) = rule.filters.protocols.as_ref() {
+            ensure_max_len(&protocols, "filters.protocols", MAX_FW_RULE_PARTS)?;
+        }


If a customer is exceeding 2 or more of these limits, it'd be really nice to get a single error message that tells them every limit they've exceeded. WDYT?

nexus/db-model/src/vpc_firewall_rule.rs

david-crespo · 2024-07-01T00:10:56Z

Will add a line to the descriptions, and where possible add a max length to the OpenAPI schema.

david-crespo · 2024-07-17T22:14:06Z

openapi/nexus.json

@@ -20438,7 +20439,7 @@
        ]
      },
      "VpcFirewallRuleFilter": {
-        "description": "Filter for a firewall rule. A given packet must match every field that is present for the rule to apply to it. A packet matches a field if any entry in that field matches the packet.",
+        "description": "Filters reduce the scope of a firewall rule. Without filters, the rule applies to all packets to the targets (or from the targets, if it's an outbound rule). With multiple filters, the rule applies only to packets matching ALL filters. The maximum number of each type of filter is 256.",


@karencfv I brought in something more like the console explanation here. I think it's an improvement, the only issue is that it's hard to find on the docs site, buried in the request body.

What might be nicer would be to put some basic summary of all of it here at the top... OR we could just link to the guide, which is pretty nice: https://docs.oxide.computer/guides/configuring-guest-networking#_firewall_rules

Oh nice! This is great. I'd like to keep this here so the SDKs inherit the descriptions, but it would be really great to add the text from the second image as well for the docs. Would it be too weird to have it all?

Yeah, I’ll add more. I don’t think anyone will be mad at us for having too good of documentation.

karencfv · 2024-07-18T09:10:55Z

nexus/db-model/src/vpc_firewall_rule.rs

+const MAX_FW_RULES_PER_VPC: usize = 1024;
+
+/// Cap on targets and on each type of filter
+const MAX_FW_RULE_PARTS: usize = 256;


Just occurred to me. We don't support port ranges (e.g. "3000-4000") as a value, do we? If so, we should increase this number. Or better yet, add support for port ranges at some point.

If we do support port ranges, then ignore this comment :)

We do support ranges. You just put in “123-456”.

Sweet! Can we add that to the API docs?

It's sort of in there, but I will improve the wording.

karencfv · 2024-07-18T09:15:40Z

openapi/nexus.json

@@ -20438,7 +20439,7 @@
        ]
      },
      "VpcFirewallRuleFilter": {
-        "description": "Filter for a firewall rule. A given packet must match every field that is present for the rule to apply to it. A packet matches a field if any entry in that field matches the packet.",
+        "description": "Filters reduce the scope of a firewall rule. Without filters, the rule applies to all packets to the targets (or from the targets, if it's an outbound rule). With multiple filters, the rule applies only to packets matching ALL filters. The maximum number of each type of filter is 256.",


Oh nice! This is great. I'd like to keep this here so the SDKs inherit the descriptions, but it would be really great to add the text from the second image as well for the docs. Would it be too weird to have it all?

david-crespo · 2024-07-24T21:57:20Z

I've added a nice description to the update endpoint that includes some of the explanation. Note that we need oxidecomputer/dropshot#1068 and a CSS tweak in the docs site to make this look good, but it's not awful as-is.

What it will look like if we don't do anything else

What it will look like after we fix a couple of things in dropshot and the docs site

david-crespo added the api Related to the API. label Jun 18, 2024

karencfv reviewed Jun 18, 2024

View reviewed changes

Base automatically changed from firewall-same-name-500 to main June 19, 2024 19:45

david-crespo force-pushed the max-firewall-rules branch 2 times, most recently from a4b3ba0 to 75b2ea8 Compare June 19, 2024 22:27

raise cap to 512 and use it for targets and filters too

a9a8755

david-crespo force-pushed the max-firewall-rules branch from 75b2ea8 to a9a8755 Compare June 19, 2024 22:42

david-crespo marked this pull request as ready for review June 28, 2024 15:24

david-crespo added 2 commits June 28, 2024 10:46

split cap into rules cap (1024) and parts cap (128)

27bd658

Merge branch 'main' into max-firewall-rules

e76d4e6

karencfv reviewed Jun 30, 2024

View reviewed changes

david-crespo added 2 commits July 17, 2024 15:01

Merge main into max-firewall-rules

b6932bc

increase parts cap to 256 and update various doc comments

5bdaedc

david-crespo commented Jul 17, 2024

View reviewed changes

karencfv reviewed Jul 18, 2024

View reviewed changes

david-crespo added 2 commits July 23, 2024 17:32

Merge branch 'main' into max-firewall-rules

d02e474

more detailed description in API rule replace endpoint

8d69b62

david-crespo enabled auto-merge (squash) July 24, 2024 22:18

david-crespo mentioned this pull request Jul 24, 2024

Preserve blank lines in endpoint descriptions oxidecomputer/dropshot#1068

Merged

had to update the sled agent API JSON for the first time in my dang life

a1ecf5a

david-crespo merged commit 71fe60a into main Jul 25, 2024
23 checks passed

david-crespo deleted the max-firewall-rules branch July 25, 2024 06:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit number of firewall rules per VPC #5914

Limit number of firewall rules per VPC #5914

david-crespo commented Jun 18, 2024 •

edited

Loading

karencfv Jun 18, 2024

rmustacc Jun 18, 2024

david-crespo Jun 18, 2024

karencfv left a comment

karencfv Jun 30, 2024

david-crespo commented Jul 1, 2024

david-crespo Jul 17, 2024

karencfv Jul 18, 2024

david-crespo Jul 18, 2024

karencfv Jul 18, 2024

david-crespo Jul 18, 2024

karencfv Jul 19, 2024 •

edited

Loading

david-crespo Jul 19, 2024

karencfv Jul 18, 2024

david-crespo commented Jul 24, 2024 •

edited

Loading

Limit number of firewall rules per VPC #5914

Limit number of firewall rules per VPC #5914

Conversation

david-crespo commented Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karencfv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-crespo commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karencfv Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-crespo commented Jul 24, 2024 • edited Loading

What it will look like if we don't do anything else

What it will look like after we fix a couple of things in dropshot and the docs site

david-crespo commented Jun 18, 2024 •

edited

Loading

karencfv Jul 19, 2024 •

edited

Loading

david-crespo commented Jul 24, 2024 •

edited

Loading